Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TT-13185] Implement Password Flow OAuth #6649

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

andrei-tyk
Copy link
Contributor

@andrei-tyk andrei-tyk commented Oct 16, 2024

User description

Ticket link: https://tyktech.atlassian.net/browse/TT-13185

Description

This PR implements an OAuth 2.0 authentication mechanism within the Tyk API Gateway, using the Password Flow to securely authenticate and authorize requests forwarded to the Upstream server. It involves configuring the Gateway to obtain, manage, and refresh access tokens, ensuring secure communication between the Gateway and the Upstream services.

Related Issue

This PR is related to the issue of enhancing security for the API Gateway, which previously forwarded requests directly to the Upstream server without authenticating itself. This introduces an OAuth 2.0 mechanism to address the security concerns. A Jira ticket has been created to track this issue.

Motivation and Context

The change is necessary to ensure that only authenticated requests reach the Upstream server. By implementing OAuth 2.0 Password Flow, the API Gateway will now authenticate itself before forwarding requests, securing communication between the Gateway and the Upstream server. This solves the problem of unauthorized requests being forwarded and strengthens overall security.

How This Has Been Tested

Unit tests have been written to verify the OAuth token retrieval, caching, and refresh mechanisms.
The API Gateway was tested in an environment using a mock Upstream server protected by OAuth (using Keycloak).
Simulated token expiration scenarios were tested to ensure the Gateway properly handles token refreshes and retries the request.
Error handling scenarios were tested for cases such as invalid credentials or network failures.
The Redis store was checked to verify that tokens are hashed (RSA256 - FIPS compliant) before being stored, and no token appears in the logs.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Enhancement, Tests


Description

  • Implemented OAuth2 password flow in the Tyk API Gateway, enhancing security by authenticating requests before forwarding them to upstream servers.
  • Added PasswordAuthentication struct to configure OAuth2 password flow, including username, password, and token URL.
  • Introduced PasswordOAuthProvider to manage token retrieval and caching for password flow.
  • Developed unit tests to verify the functionality of the OAuth2 password flow, ensuring proper token handling and error management.

Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Add OAuth2 Password Flow Configuration to API Definitions

apidef/api_definitions.go

  • Added PasswordAuthentication struct for OAuth2 password flow
    configuration.
  • Included PasswordAuthentication in UpstreamOAuth.
  • +20/-0   
    upstream.go
    Extend Upstream OAuth to Support Password Authentication 

    apidef/oas/upstream.go

  • Added PasswordAuthentication struct and methods.
  • Extended UpstreamOAuth to support password authentication.
  • +60/-3   
    mw_oauth2_auth.go
    Implement OAuth2 Password Flow in Middleware                         

    gateway/mw_oauth2_auth.go

  • Implemented PasswordOAuthProvider for handling password flow.
  • Added caching mechanism for password flow tokens.
  • +104/-5 
    Tests
    mw_oauth2_auth_test.go
    Add Tests for OAuth2 Password Flow Implementation               

    gateway/mw_oauth2_auth_test.go

  • Added tests for OAuth2 password credentials token request.
  • Verified token retrieval and header setting.
  • +82/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    andrei-tyk and others added 25 commits October 14, 2024 09:04
    … refactoring to allow for easier extension of the oauth flows
    # Conflicts:
    #	apidef/api_definitions.go
    #	apidef/oas/upstream.go
    #	gateway/mw_oauth2_auth.go
    #	gateway/mw_oauth2_auth_test.go
    #	gateway/server.go
    …-password-flow
    
    # Conflicts:
    #	apidef/api_definitions.go
    #	apidef/oas/upstream.go
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The implementation includes direct handling of passwords which are sensitive data. Ensure that passwords are not logged or exposed in error messages. Consider implementing additional security measures such as encryption or secure storage mechanisms.

    ⚡ Recommended focus areas for review

    Security Concern
    The implementation of password-based OAuth should ensure that sensitive data such as passwords are securely handled. The current implementation exposes the password in the source code, which could lead to security vulnerabilities.

    Data Exposure
    Passwords are included in the configuration struct which might get logged or exposed through debugging. Ensure that sensitive fields are omitted from logs and error messages.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Improve error handling in encryption and decryption to enhance robustness

    Add error handling for potential failures in the encrypt and decrypt functions to
    prevent runtime panics.

    gateway/mw_oauth2_auth.go [64-73]

    -decryptedToken := decrypt(getPaddedSecret(OAuthSpec.Gw), tokenString)
    -encryptedToken := encrypt(getPaddedSecret(OAuthSpec.Gw), token.AccessToken)
    +decryptedToken, err := decrypt(getPaddedSecret(OAuthSpec.Gw), tokenString)
    +if err != nil {
    +  return "", err
    +}
    +encryptedToken, err := encrypt(getPaddedSecret(OAuthSpec.Gw), token.AccessToken)
    +if err != nil {
    +  return "", err
    +}
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the encrypt and decrypt functions is a significant improvement. It prevents potential runtime panics and ensures that any issues during these operations are properly managed, enhancing the overall reliability of the code.

    9
    Validate token expiry to prevent caching invalid tokens

    Validate the token.Expiry to ensure it is a future timestamp before setting the TTL
    for the cache.

    gateway/mw_oauth2_auth.go [75-77]

    +if token.Expiry.Before(time.Now()) {
    +  return "", fmt.Errorf("invalid token expiry")
    +}
     ttl := time.Until(token.Expiry)
     if err := setTokenInCache(cacheKey, encryptedToken, ttl, &cache.RedisCluster); err != nil {
       return "", err
     }
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential bug by ensuring that only tokens with a valid future expiry are cached. This prevents caching expired tokens, which could lead to authentication failures, thus improving the robustness of the token handling logic.

    8
    Performance
    Prevent excessive token requests by implementing rate limiting or caching

    Implement rate limiting or caching mechanisms to avoid excessive token generation
    requests, potentially leading to service abuse.

    gateway/mw_oauth2_auth.go [86]

    -token, err := cfg.PasswordCredentialsToken(ctx, OAuthSpec.Spec.UpstreamAuth.OAuth.PasswordAuthentication.Username, OAuthSpec.Spec.UpstreamAuth.OAuth.PasswordAuthentication.Password)
    +// Implement rate limiting or use an existing token from cache if valid
    +token, err := checkCachedOrGenerateNewToken(ctx, cfg, OAuthSpec)
    Suggestion importance[1-10]: 7

    Why: The suggestion to implement rate limiting or caching mechanisms is relevant for preventing service abuse and improving performance. However, the suggestion lacks specific implementation details, which slightly reduces its immediate applicability.

    7

    Base automatically changed from TT-13184-upstream-oauth2 to master October 17, 2024 07:56
    // Scopes specifies optional requested permissions.
    Scopes []string `bson:"scopes" json:"scopes,omitempty"`

    Token *oauth2.Token `bson:"-" json:"-"`
    Copy link
    Contributor

    @jeffy-mathew jeffy-mathew Oct 17, 2024

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    do we need this in OAS?

    }

    if tokenString != "" {
    decryptedToken := decrypt(getPaddedSecret(OAuthSpec.Gw), tokenString)
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    getPaddedSecret seems to be using only gw.GetConfig().Secret, shall we pass only the secret as a dependency

    # Conflicts:
    #	apidef/api_definitions.go
    #	apidef/oas/schema/x-tyk-api-gateway.json
    #	gateway/mw_oauth2_auth.go
    Copy link
    Contributor

    github-actions bot commented Oct 22, 2024

    API Changes

    --- prev.txt	2024-10-23 08:59:10.815250895 +0000
    +++ current.txt	2024-10-23 08:59:04.327218653 +0000
    @@ -886,6 +886,9 @@
     						"client_credentials": {
     							"type": "object",
     							"properties": {
    +								"enabled": {
    +									"type": "boolean"
    +								},
     								"client_id": {
     									"type": "string"
     								},
    @@ -897,11 +900,41 @@
     								},
     								"scopes":{
     									"type": ["array", "null"]
    -								}	
    +								},
    +								"header_name": {
    +									"type": "string"
    +								}
     							}
     						},
    -						"header_name": {
    -							"type": "string"		
    +						"password_authentication": {
    +						  "type": "object",
    +						  "properties": {
    +								"enabled": {
    +								  "type": "boolean"
    +								},
    +								"client_id": {
    +								  "type": "string"
    +								},
    +								"client_secret": {
    +								  "type": "string"
    +								},
    +								"username": {
    +								  "type": "string"
    +								},
    +								"password": {
    +								  "type": "string"
    +								},
    +								"token_url": {
    +								  "type": "string"
    +								},
    +								"scopes": {
    +								  "type": ["array", "null"]
    +								},
    +								"header_name": {
    +								  "type": "string"
    +								}
    +							}
    +						  }
     						}
     					}
     				}
    @@ -1218,11 +1251,16 @@
     
     type ClientCredentials struct {
     	ClientAuthData
    +	// Enabled activates upstream OAuth2 client credentials authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
     	// TokenURL is the resource server's token endpoint
     	// URL. This is a constant specific to each server.
     	TokenURL string `bson:"token_url" json:"token_url"`
     	// Scopes specifies optional requested permissions.
     	Scopes []string `bson:"scopes" json:"scopes,omitempty"`
    +	// HeaderName is the custom header name to be used for OAuth client credential flow authentication.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"header_name" json:"header_name"`
     
     	// TokenProvider is the OAuth2 token provider for internal use.
     	TokenProvider oauth2.TokenSource `bson:"-" json:"-"`
    @@ -1716,6 +1754,29 @@
     	SegregateByClient bool                `bson:"segregate_by_client" json:"segregate_by_client"`
     }
     
    +type PasswordAuthentication struct {
    +	ClientAuthData
    +	// Enabled activates upstream OAuth2 password authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// Username is the username to be used for upstream OAuth2 password authentication.
    +	Username string `bson:"username" json:"username"`
    +	// Password is the password to be used for upstream OAuth2 password authentication.
    +	Password string `bson:"password" json:"password"`
    +	// TokenURL is the resource server's token endpoint
    +	// URL. This is a constant specific to each server.
    +	TokenURL string `bson:"token_url" json:"token_url"`
    +	// Scopes specifies optional requested permissions.
    +	Scopes []string `bson:"scopes" json:"scopes,omitempty"`
    +	// HeaderName is the custom header name to be used for OAuth password authentication flow.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"header_name" json:"header_name"`
    +
    +	// TokenProvider is the OAuth2 password authentication flow token for internal use.
    +	Token *oauth2.Token `bson:"-" json:"-"`
    +}
    +    PasswordAuthentication holds the configuration for upstream OAuth2 password
    +    authentication flow.
    +
     type PersistGraphQLMeta struct {
     	Path      string                 `bson:"path" json:"path"`
     	Method    string                 `bson:"method" json:"method"`
    @@ -2078,9 +2139,8 @@
     	Enabled bool `bson:"enabled" json:"enabled"`
     	// ClientCredentials holds the client credentials for upstream OAuth2 authentication.
     	ClientCredentials ClientCredentials `bson:"client_credentials" json:"client_credentials"`
    -	// HeaderName is the custom header name to be used for upstream basic authentication.
    -	// Defaults to `Authorization`.
    -	HeaderName string `bson:"header_name" json:"header_name,omitempty"`
    +	// PasswordAuthentication holds the configuration for upstream OAauth password authentication flow.
    +	PasswordAuthentication PasswordAuthentication `bson:"password_authentication,omitempty" json:"passwordAuthentication,omitempty"`
     }
         UpstreamOAuth holds upstream OAuth2 authentication configuration.
     
    @@ -3104,6 +3164,14 @@
     func (cb *CircuitBreaker) Fill(circuitBreaker apidef.CircuitBreakerMeta)
         Fill fills *CircuitBreaker from apidef.CircuitBreakerMeta.
     
    +type ClientAuthData struct {
    +	// ClientID is the application's ID.
    +	ClientID string `bson:"clientId" json:"clientId"`
    +	// ClientSecret is the application's secret.
    +	ClientSecret string `bson:"clientSecret" json:"clientSecret"`
    +}
    +    ClientAuthData holds the client ID and secret for OAuth2 authentication.
    +
     type ClientCertificates struct {
     	// Enabled activates static mTLS for the API.
     	Enabled bool `bson:"enabled" json:"enabled"`
    @@ -3120,15 +3188,17 @@
         Fill fills *ClientCertificates from apidef.APIDefinition.
     
     type ClientCredentials struct {
    -	// ClientID is the application's ID.
    -	ClientID string `bson:"clientID" json:"clientID"`
    -	// ClientSecret is the application's secret.
    -	ClientSecret string `bson:"clientSecret" json:"clientSecret"`
    +	ClientAuthData
    +	// Enabled activates upstream OAuth2 client credentials authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
     	// TokenURL is the resource server's token endpoint
     	// URL. This is a constant specific to each server.
     	TokenURL string `bson:"tokenURL" json:"tokenURL"`
     	// Scopes specifies optional requested permissions.
     	Scopes []string `bson:"scopes,omitempty" json:"scopes,omitempty"`
    +	// HeaderName is the custom header name to be used for OAuth client credential flow authentication.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"headerName" json:"headerName"`
     }
         ClientCredentials holds the configuration for OAuth2 Client Credentials
         flow.
    @@ -4029,6 +4099,30 @@
     type Operations map[string]*Operation
         Operations holds Operation definitions.
     
    +type PasswordAuthentication struct {
    +	ClientAuthData
    +	// Enabled activates upstream OAuth2 password authentication.
    +	Enabled bool `bson:"enabled" json:"enabled"`
    +	// Username is the username to be used for upstream OAuth2 password authentication.
    +	Username string `bson:"username" json:"username"`
    +	// Password is the password to be used for upstream OAuth2 password authentication.
    +	Password string `bson:"password" json:"password"`
    +	// TokenURL is the resource server's token endpoint
    +	// URL. This is a constant specific to each server.
    +	TokenURL string `bson:"tokenURL" json:"tokenURL"`
    +	// Scopes specifies optional requested permissions.
    +	Scopes []string `bson:"scopes" json:"scopes,omitempty"`
    +	// HeaderName is the custom header name to be used for OAuth password authentication flow.
    +	// Defaults to `Authorization`.
    +	HeaderName string `bson:"headerName" json:"headerName"`
    +}
    +    PasswordAuthentication holds the configuration for upstream OAuth2 password
    +    authentication flow.
    +
    +func (p *PasswordAuthentication) ExtractTo(api *apidef.PasswordAuthentication)
    +
    +func (p *PasswordAuthentication) Fill(api apidef.PasswordAuthentication)
    +
     type Path struct {
     	// Delete holds plugin configuration for DELETE requests.
     	Delete *Plugins `bson:"DELETE,omitempty" json:"DELETE,omitempty"`
    @@ -4884,9 +4978,8 @@
     	Enabled bool `bson:"enabled" json:"enabled"`
     	// ClientCredentials holds the configuration for OAuth2 Client Credentials flow.
     	ClientCredentials *ClientCredentials `bson:"clientCredentials,omitempty" json:"clientCredentials,omitempty"`
    -	// HeaderName is the custom header name to be used for upstream basic authentication.
    -	// Defaults to `Authorization`.
    -	HeaderName string `bson:"headerName" json:"headerName"`
    +	// PasswordAuthentication holds the configuration for upstream OAauth password authentication flow.
    +	PasswordAuthentication *PasswordAuthentication `bson:"passwordAuthentication,omitempty" json:"passwordAuthentication,omitempty"`
     }
         UpstreamOAuth holds the configuration for OAuth2 Client Credentials flow.
     
    @@ -9904,6 +9997,8 @@
     
     func (k *OrganizationMonitor) SetOrgSentinel(orgChan chan bool, orgId string)
     
    +type PasswordOAuthProvider struct{}
    +
     type PerAPIClientCredentialsOAuthProvider struct{}
     
     type PersistGraphQLOperationMiddleware struct {

    }

    switch {
    case oauthConfig.ClientCredentials.ClientID != "" && oauthConfig.PasswordAuthentication.ClientID != "":
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    enabled flag needs to be respected here

    @andrei-tyk andrei-tyk changed the title [Tt 13185] Implement Password Flow OAuth [TT-13185] Implement Password Flow OAuth Oct 22, 2024
    w.Header().Set("Content-Type", "application/x-www-form-urlencoded")
    w.Write([]byte("access_token=90d64460d14870c08c81352a05dedd3465940a7c&scope=user&token_type=bearer"))
    }))
    defer ts.Close()
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Also t.Cleanup

    defer r.Body.Close()
    expected := "/token"
    if r.URL.String() != expected {
    t.Errorf("URL = %q; want %q", r.URL, expected)
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Use testify/assert in here (already used below)

    // ClientID is the application's ID.
    ClientID string `bson:"clientID" json:"clientID"`
    ClientID string `bson:"clientId" json:"clientId"`
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Breaking change?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    that was some unintended typo , wil revert it to the one agreed in the schema

    Copy link

    sonarcloud bot commented Oct 23, 2024

    Quality Gate Failed Quality Gate failed

    Failed conditions
    0.0% Coverage on New Code (required ≥ 80%)
    C Reliability Rating on New Code (required ≥ A)

    See analysis details on SonarCloud

    Catch issues before they fail your Quality Gate with our IDE extension SonarLint

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants